fix: resolve critical bugs in config, transactions, and UI #3
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit fixes several critical bugs discovered during testing: ## Bug Fixes ### 1. Config chains edit/add removes transactionServiceUrl - **Problem:** When editing or adding chains, the transactionServiceUrl and contractNetworks fields were being removed from all chains - **Fix:** Updated editChains() to preserve all ChainConfig fields when saving - **Fix:** Added prompt for transactionServiceUrl when adding new chains - **Files:** src/commands/config/chains.ts, src/commands/config/edit.ts ### 2. Transaction list shows "Signatures: 0/undefined" - **Problem:** Threshold was showing as "undefined" because it was stored locally but could become stale when changed on-chain - **Fix:** Changed architecture to always fetch threshold and owners fresh from blockchain via RPC instead of storing them - **Files:** Multiple tx commands (sign, execute, status, create, etc.) ### 3. Transaction sign fails with "Cannot read properties of null" - **Problem:** Multiple sources of null values causing crashes: - transaction.signatures array could be null for legacy transactions - metadata.value, metadata.data, metadata.operation could be null - **Fix:** Added normalization layer in TransactionStore to ensure signatures array is always initialized - **Fix:** Added default values for metadata fields in signTransaction() and executeTransaction() - **Fix:** Protected all array method calls with optional chaining and fallbacks - **Files:** src/storage/transaction-store.ts, src/services/transaction-service.ts, all tx commands ### 4. Duplicate variable declarations in create.ts - **Problem:** Duplicate chain and spinner variable declarations causing compilation errors - **Fix:** Removed duplicate chain declaration and renamed second spinner to spinner2 - **File:** src/commands/tx/create.ts ## UI Improvements ### Update Safe Transaction Service link to Safe Wallet - Changed "View on Safe Transaction Service" to "View in Safe Wallet" - Updated URL format to use Safe Wallet app: https://app.safe.global/transactions/tx - **Files:** src/ui/screens/TransactionPushSuccessScreen.tsx, src/commands/tx/push.ts ## Technical Details - All commands now fetch owners/threshold from blockchain for accuracy - Transaction storage now normalizes data on retrieval - All array operations protected against null/undefined values - Metadata fields have safe defaults 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR addresses critical bugs in the Safe CLI application related to configuration management, transaction handling, and UI display. The changes focus on data integrity, null safety, and fetching live blockchain state to prevent stale data issues.
Key Changes:
- Added normalization layer to ensure transaction signatures arrays are never null
- Updated all transaction commands to fetch owners/threshold directly from blockchain via RPC instead of using cached values
- Fixed config edit operations to preserve all chain configuration fields including
transactionServiceUrlandcontractNetworks
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| TransactionPushSuccessScreen.tsx | Updated UI text from "Safe Transaction Service" to "Safe Wallet" |
| TransactionListScreen.tsx | Added live threshold fetching from blockchain with fallback handling |
| transaction-store.ts | Implemented signature array normalization for null safety |
| transaction-service.ts | Added default values for metadata fields to prevent null errors |
| tx/sync.ts | Protected signature array operations with null coalescing |
| tx/status.ts | Replaced cached owner/threshold with live blockchain fetch |
| tx/sign.ts | Refactored to fetch live Safe data and fixed duplicate spinner variable |
| tx/push.ts | Protected signature operations and updated service URL format |
| tx/pull.ts | Added null safety for signature array operations |
| tx/import.ts | Protected signature length checks with null coalescing |
| tx/export.ts | Added null safety for signature count display |
| tx/execute.ts | Refactored to fetch live Safe data and renamed duplicate spinner |
| tx/create.ts | Refactored to fetch live owners and renamed duplicate spinner |
| config/edit.ts | Added transactionServiceUrl and contractNetworks to preserved fields |
| config/chains.ts | Added prompt for transactionServiceUrl when adding chains |
| account/remove-owner.ts | Replaced cached data with live blockchain fetch, renamed spinner |
| account/change-threshold.ts | Replaced cached data with live blockchain fetch, renamed spinner |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| transaction.chainId, | ||
| chains | ||
| ) | ||
| const serviceUrl = `https://app.safe.global/transactions/tx?safe=${chain.shortName}:${transaction.safeAddress}&id=${selectedSafeTxHash}` |
There was a problem hiding this comment.
The Safe Wallet URL format is incorrect. According to the EIP-3770 format used elsewhere in the codebase, the safe parameter should use the full EIP-3770 address which is already computed in the eip3770 variable on line 167, not a manual concatenation of chain.shortName and transaction.safeAddress. Use ${eip3770} instead of ${chain.shortName}:${transaction.safeAddress}.
Suggested change
| const serviceUrl = `https://app.safe.global/transactions/tx?safe=${chain.shortName}:${transaction.safeAddress}&id=${selectedSafeTxHash}` | |
| const serviceUrl = `https://app.safe.global/transactions/tx?safe=${eip3770}&id=${selectedSafeTxHash}` |
Comment on lines
+82
to
+83
| } catch { | ||
| // Silently fail - threshold will remain undefined |
There was a problem hiding this comment.
The error is being silently swallowed without logging. Users should be informed when threshold cannot be fetched from the blockchain. Consider logging the error with console.error() or similar to aid debugging while still preventing UI crashes.
Suggested change
| } catch { | |
| // Silently fail - threshold will remain undefined | |
| } catch (error) { | |
| console.error( | |
| `Failed to fetch threshold for Safe ${transaction.safeAddress} on chain ${transaction.chainId}:`, | |
| error | |
| ) | |
| // threshold will remain undefined |
| currency: 'Native currency symbol (e.g., "ETH")', | ||
| explorer: '(Optional) Block explorer base URL', | ||
| transactionServiceUrl: '(Optional) Safe Transaction Service API URL', | ||
| contractNetworks: '(Optional) Safe contract addresses for this chain', |
There was a problem hiding this comment.
The help text for contractNetworks is unclear about the expected format. Based on the ChainConfig type, this should be an object with contract addresses. Consider clarifying the format, e.g., '(Optional) Safe contract addresses object for this chain (e.g., {"1.3.0": "0x..."})'.
Suggested change
| contractNetworks: '(Optional) Safe contract addresses for this chain', | |
| contractNetworks: '(Optional) Safe contract addresses object for this chain (e.g., {"1.3.0": "0x..."})', |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes several critical bugs discovered during testing that were causing crashes and incorrect behavior.
Bug Fixes
1. Config chains edit/add removes transactionServiceUrl ✅
transactionServiceUrlandcontractNetworksfields were being removed from all chainseditChains()to preserve all ChainConfig fields when savingtransactionServiceUrlwhen adding new chains2. Transaction list shows "Signatures: 0/undefined" ✅
3. Transaction sign fails with "Cannot read properties of null" ✅
transaction.signaturesarray could be null for legacy transactionsmetadata.value,metadata.data,metadata.operationcould be nullTransactionStoreto ensure signatures array is always initializedsignTransaction()andexecuteTransaction()4. Duplicate variable declarations in create.ts ✅
chainandspinnervariable declarations causing compilation errorschaindeclaration and renamed secondspinnertospinner2UI Improvements
Update Safe Transaction Service link to Safe Wallet ✅
https://app.safe.global/transactions/tx?safe=<eip3770>&id=<txHash>Technical Details
Architecture Changes
Files Changed (17 files)
Testing
Breaking Changes
None - all changes are backward compatible with legacy transaction data.
🤖 Generated with Claude Code